Flow.Launcher/Helper/KeyboardLayoutHelper.cs (4)

1-66: Good implementation of Windows APIs for keyboard layout management

The implementation correctly uses P/Invoke to access the necessary Windows APIs for keyboard layout management. The FindEnglishKeyboardLayout method effectively retrieves and filters keyboard layouts to identify English ones.

However, the current language detection method could be improved:

Consider using explicit English language IDs instead of string comparison for more robust language detection. The current approach using string contains "english" might not work reliably across all Windows versions and language configurations.

-    private const string LAYOUT_ENGLISH_SEARCH = "english";
+    // English language ID (0x0409 = US English, 0x0809 = UK English, etc.)
+    private static readonly uint[] ENGLISH_LANGUAGE_IDS = { 0x0409, 0x0809, 0x0c09, 0x1009, 0x1409, 0x1809, 0x1c09, 0x2009, 0x2409, 0x2809, 0x2c09, 0x3009, 0x3409 };

     private static IntPtr FindEnglishKeyboardLayout()
     {
         // Get the number of keyboard layouts
         var count = GetKeyboardLayoutList(0, null);
         if (count <= 0) return IntPtr.Zero;

         // Get all keyboard layouts
         var keyboardLayouts = new IntPtr[count];
         GetKeyboardLayoutList(count, keyboardLayouts);

         // Look for any English keyboard layout
         foreach (var layout in keyboardLayouts)
         {
             // The lower word contains the language identifier
             var langId = (uint)layout.ToInt32() & 0xFFFF;
-
-            // Get language name for the layout
-            var sb = new StringBuilder(256);
-            GetLocaleInfoA(langId, LOCALE_SLANGUAGE, sb, sb.Capacity);
-            var langName = sb.ToString().ToLowerInvariant();
-
-            // Check if it's an English layout
-            if (langName.Contains(LAYOUT_ENGLISH_SEARCH))
+            // Check if it's an English language ID
+            if (Array.IndexOf(ENGLISH_LANGUAGE_IDS, langId) >= 0)
             {
                 return layout;
             }
         }

         return IntPtr.Zero;
     }

---

68-87: Consider adding thread safety to the layout switching mechanism

The _previousLayout static field is shared and could cause race conditions if layout switching is triggered from multiple threads simultaneously.

Add thread synchronization to prevent potential race conditions:

-    private static IntPtr _previousLayout;
+    private static IntPtr _previousLayout;
+    private static readonly object _layoutLock = new object();

     public static void SetEnglishKeyboardLayout()
     {
-        // Find an installed English layout
-        var englishLayout = FindEnglishKeyboardLayout();
-
-        // No installed English layout found
-        if (englishLayout == IntPtr.Zero) return;
-
-        var hwnd = GetForegroundWindow();
-        var threadId = GetWindowThreadProcessId(hwnd, IntPtr.Zero);
-
-        // Store current keyboard layout
-        _previousLayout = GetKeyboardLayout(threadId) & 0xFFFF;
-
-        // Switch to English layout
-        ActivateKeyboardLayout(englishLayout, 0);
+        lock (_layoutLock)
+        {
+            // Find an installed English layout
+            var englishLayout = FindEnglishKeyboardLayout();
+
+            // No installed English layout found
+            if (englishLayout == IntPtr.Zero) return;
+
+            var hwnd = GetForegroundWindow();
+            var threadId = GetWindowThreadProcessId(hwnd, IntPtr.Zero);
+
+            // Store current keyboard layout
+            _previousLayout = GetKeyboardLayout(threadId);
+
+            // Switch to English layout
+            ActivateKeyboardLayout(englishLayout, 0);
+        }
     }

Also, the bitwise AND operation with 0xFFFF might be unnecessary when storing the layout handle. This operation extracts only the language ID portion but discards the keyboard type information.

---

89-94: Apply thread safety to SetPreviousKeyboardLayout method as well

Similar to the SetEnglishKeyboardLayout method, this method should also be thread-safe.

     public static void SetPreviousKeyboardLayout()
     {
-        if (_previousLayout == IntPtr.Zero) return;
-        ActivateKeyboardLayout(_previousLayout, 0);
-        _previousLayout = IntPtr.Zero;
+        lock (_layoutLock)
+        {
+            if (_previousLayout == IntPtr.Zero) return;
+            ActivateKeyboardLayout(_previousLayout, 0);
+            _previousLayout = IntPtr.Zero;
+        }
     }

---

7-95: Add error handling and logging for diagnostics

Since this is an experimental feature and involves Windows API calls that might fail, consider adding error handling and logging.

Add try-catch blocks and logging to help with debugging:

     public static void SetEnglishKeyboardLayout()
     {
+        try
+        {
             // Find an installed English layout
             var englishLayout = FindEnglishKeyboardLayout();

             // No installed English layout found
             if (englishLayout == IntPtr.Zero) return;

             var hwnd = GetForegroundWindow();
             var threadId = GetWindowThreadProcessId(hwnd, IntPtr.Zero);

             // Store current keyboard layout
             _previousLayout = GetKeyboardLayout(threadId) & 0xFFFF;

             // Switch to English layout
             ActivateKeyboardLayout(englishLayout, 0);
+        }
+        catch (Exception ex)
+        {
+            // Log the exception but don't crash the application
+            Log.Error("KeyboardLayoutHelper", $"Error switching to English layout: {ex.Message}", ex);
+        }
     }

     public static void SetPreviousKeyboardLayout()
     {
+        try
+        {
             if (_previousLayout == IntPtr.Zero) return;
             ActivateKeyboardLayout(_previousLayout, 0);
             _previousLayout = IntPtr.Zero;
+        }
+        catch (Exception ex)
+        {
+            // Log the exception but don't crash the application
+            Log.Error("KeyboardLayoutHelper", $"Error restoring previous layout: {ex.Message}", ex);
+        }
     }

Also consider adding a simple way to detect if an English layout is already active to avoid unnecessary switching:

public static bool IsEnglishLayoutActive()
{
    try
    {
        var hwnd = GetForegroundWindow();
        var threadId = GetWindowThreadProcessId(hwnd, IntPtr.Zero);
        var currentLayout = GetKeyboardLayout(threadId);
        var langId = (uint)currentLayout.ToInt32() & 0xFFFF;
        
        // Check if it's already an English language ID
        return Array.IndexOf(ENGLISH_LANGUAGE_IDS, langId) >= 0;
    }
    catch (Exception ex)
    {
        Log.Error("KeyboardLayoutHelper", $"Error checking if English layout is active: {ex.Message}", ex);
        return false;
    }
}

Flow.Launcher/ViewModel/MainViewModel.cs (2)

1377-1382: Good integration of keyboard layout switching on window show

The implementation correctly calls KeyboardLayoutHelper.SetEnglishKeyboardLayout() when StartWithEnglishMode is true, which fulfills the PR objective.

Consider adding a check to avoid unnecessary layout switching if English is already the active layout:

                if (StartWithEnglishMode)
                {
+                   // Only switch if not already using English layout
+                   if (!KeyboardLayoutHelper.IsEnglishLayoutActive())
                    KeyboardLayoutHelper.SetEnglishKeyboardLayout();
                }

This would require adding the IsEnglishLayoutActive() method to KeyboardLayoutHelper as suggested in the previous comment.

---

1377-1455: Consider adding exception handling for keyboard layout operations

Since keyboard layout operations involve native API calls that could potentially fail, adding try-catch blocks would make the code more robust.

Add try-catch blocks around keyboard layout operations to prevent exceptions from affecting the main application flow:

                if (StartWithEnglishMode)
                {
+                   try
+                   {
                        KeyboardLayoutHelper.SetEnglishKeyboardLayout();
+                   }
+                   catch (Exception ex)
+                   {
+                       Log.Error("MainViewModel", $"Error setting English keyboard layout: {ex.Message}", ex);
+                       // Continue execution despite the error
+                   }
                }

And similarly for the hide method:

            if (StartWithEnglishMode)
            {
+               try
+               {
                    KeyboardLayoutHelper.SetPreviousKeyboardLayout();
+               }
+               catch (Exception ex)
+               {
+                   Log.Error("MainViewModel", $"Error restoring previous keyboard layout: {ex.Message}", ex);
+                   // Continue execution despite the error
+               }
            }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Flow.Launcher.Infrastructure/Win32Helper.cs (5)

326-331: Document the English language IDs

The array contains language IDs for different English variants, but it would be helpful to add a comment explaining what some of these values represent (e.g., US English, UK English, etc.) for better maintainability.

// https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-lcid/70feba9f-294e-491e-b6eb-56532684c37f
private static readonly uint[] EnglishLanguageIds =
{
-    0x0009, 0x0409, 0x0809, 0x0C09, 0x1009, 0x1409, 0x1809, 0x1C09, 0x2009, 0x2409, 0x2809, 0x2C09, 0x3009,
-    0x3409, 0x3C09, 0x4009, 0x4409, 0x4809, 0x4C09,
+    0x0009, // English (Neutral)
+    0x0409, // English (United States)
+    0x0809, // English (United Kingdom)
+    0x0C09, // English (Australia)
+    0x1009, // English (Canada)
+    0x1409, // English (New Zealand)
+    0x1809, // English (Ireland)
+    0x1C09, // English (South Africa)
+    0x2009, // English (Jamaica)
+    0x2409, // English (Caribbean)
+    0x2809, // English (Belize)
+    0x2C09, // English (Trinidad and Tobago)
+    0x3009, // English (Zimbabwe)
+    0x3409, // English (Philippines)
+    0x3C09, // English (Hong Kong SAR)
+    0x4009, // English (India)
+    0x4409, // English (Malaysia)
+    0x4809, // English (Singapore)
+    0x4C09, // English (Nigeria)
};

---

333-336: Document the IME language IDs

Similar to the English language IDs, adding comments to describe what languages these IME IDs represent would improve code maintainability.

private static readonly uint[] ImeLanguageIds =
{
-    0x0004, 0x7804, 0x0804, 0x1004, 0x7C04, 0x0C04, 0x1404, 0x0404, 0x0011, 0x0411, 0x0012, 0x0412,
+    0x0004, // Chinese (Neutral)
+    0x7804, // Chinese (Simplified, China)
+    0x0804, // Chinese (Simplified)
+    0x1004, // Chinese (Singapore)
+    0x7C04, // Chinese (Traditional, Hong Kong SAR)
+    0x0C04, // Chinese (Traditional, Hong Kong)
+    0x1404, // Chinese (Traditional, Macau SAR)
+    0x0404, // Chinese (Traditional, Taiwan)
+    0x0011, // Japanese (Neutral)
+    0x0411, // Japanese
+    0x0012, // Korean (Neutral)
+    0x0412, // Korean
};

---

343-370: Add error handling for GetKeyboardLayoutList

The method correctly handles the case when no keyboard layouts are found, but it doesn't check if the second call to GetKeyboardLayoutList fails. Consider adding error handling for this call as well.

private static unsafe HKL FindEnglishKeyboardLayout()
{
    // Get the number of keyboard layouts
    int count = PInvoke.GetKeyboardLayoutList(0, null);
    if (count <= 0) return HKL.Null;

    // Get all keyboard layouts
    var handles = new HKL[count];
    fixed (HKL* h = handles)
    {
-        _ = PInvoke.GetKeyboardLayoutList(count, h);
+        int result = PInvoke.GetKeyboardLayoutList(count, h);
+        if (result <= 0 || result != count)
+        {
+            throw new Win32Exception(Marshal.GetLastWin32Error());
+        }
    }

    // Look for any English keyboard layout
    foreach (var hkl in handles)
    {
        // The lower word contains the language identifier
        var langId = (uint)hkl.Value & KeyboardLayoutLoWord;

        // Check if it's an English layout
        if (EnglishLanguageIds.Contains(langId))
        {
            return hkl;
        }
    }

    return HKL.Null;
}

---

372-402: Add XML documentation for public method

Since SwitchToEnglishKeyboardLayout is a public method, it should have XML documentation to describe its purpose, parameters, and any exceptions it may throw.

+/// <summary>
+/// Switches the keyboard layout to English if available.
+/// </summary>
+/// <param name="backupPrevious">If true, the current keyboard layout will be stored for later restoration.</param>
+/// <exception cref="Win32Exception">Thrown when there's an error getting the window thread process ID.</exception>
public static unsafe void SwitchToEnglishKeyboardLayout(bool backupPrevious)
{
    // Function implementation...
}

Also, the comment for IME handling is informative, but could be made more visible as part of the method documentation.

---

404-416: Add XML documentation and NULL check for GetForegroundWindow

Similar to the previous method, RestorePreviousKeyboardLayout should have XML documentation. Additionally, add a check to ensure GetForegroundWindow() doesn't return null before sending a message to it.

+/// <summary>
+/// Restores the previously backed-up keyboard layout.
+/// </summary>
public static void RestorePreviousKeyboardLayout()
{
    if (_previousLayout == HKL.Null) return;

    var hwnd = PInvoke.GetForegroundWindow();
+    if (hwnd == HWND.Null) return;
    
    PInvoke.PostMessage(
        hwnd,
        PInvoke.WM_INPUTLANGCHANGEREQUEST,
        PInvoke.INPUTLANGCHANGE_FORWARD,
        _previousLayout.Value
    );
    _previousLayout = HKL.Null;
}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Flow.Launcher.Infrastructure/Win32Helper.cs (2)

376-401: Clarify the IME handling rationale with additional comments

The code intentionally returns early when an IME layout is detected instead of switching layouts. Although there is a brief comment about this behavior, it would be helpful to provide more context about why this special handling is necessary.

Consider adding a more detailed comment like:

 // If the current layout has an IME mode, disable it without switching to another layout.
 // This is needed because for languages with IME mode, Flow Launcher just temporarily disables
 // the IME mode instead of switching to another layout.
+// IME languages require specialized input method handling and fully switching them could disrupt
+// the user experience, so we maintain the existing behavior for backward compatibility.

---

402-411: Consider adding error handling for ActivateKeyboardLayout

The call to PInvoke.ActivateKeyboardLayout might fail, but there's no error checking or handling.

Consider adding error handling:

 // Switch to English layout
-PInvoke.ActivateKeyboardLayout(enHKL, 0);
+var result = PInvoke.ActivateKeyboardLayout(enHKL, 0);
+if (result == HKL.Null)
+{
+    // Log the error or handle the failure
+    var errorCode = Marshal.GetLastWin32Error();
+    System.Diagnostics.Debug.WriteLine($"Failed to activate keyboard layout: {errorCode}");
+}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Flow.Launcher.Infrastructure/Win32Helper.cs (3)

327-336: Language ID arrays look good, consider adding a comment for selected IDs.

The implementation of the static readonly arrays for language IDs is appropriate and includes a helpful link to documentation.

Consider adding a brief comment explaining which languages are covered by the ImeLanguageIds array (Chinese variants, Japanese, Korean) to improve code readability for future maintainers.

---

384-401: Early return for IME layouts needs clearer documentation.

The code correctly identifies and handles IME layouts differently, but the comment could be more explanatory.

The comment on lines 393-395 mentions that "Flow Launcher just temporarily disables the IME mode instead of switching to another layout," but it doesn't explain why this approach is taken. Consider expanding the comment to clarify the rationale, which would help future developers understand this design decision.

---

402-411: Add error handling for ActivateKeyboardLayout.

The keyboard layout activation lacks error handling.

The call to ActivateKeyboardLayout at line 410 should include error handling to catch potential failures. Consider adding a check for the return value and throwing an exception or logging a warning if it fails:

-PInvoke.ActivateKeyboardLayout(enHKL, 0);
+var result = PInvoke.ActivateKeyboardLayout(enHKL, 0);
+if (result == HKL.Null)
+{
+    // Log warning or throw exception
+    throw new Win32Exception(Marshal.GetLastWin32Error());
+}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Flow.Launcher.Infrastructure/Win32Helper.cs (1)

417-435: 🛠️ Refactor suggestion

Use SendMessage instead of PostMessage for reliable layout restoration.

Using PostMessage for keyboard layout restoration can lead to race conditions since it's asynchronous. If another keyboard layout change occurs before the message is processed, the restoration might not work as expected.

-            PInvoke.PostMessage(
-                hwnd,
-                PInvoke.WM_INPUTLANGCHANGEREQUEST,
-                PInvoke.INPUTLANGCHANGE_FORWARD,
-                _previousLayout.Value
-            );
+            PInvoke.SendMessage(
+                hwnd,
+                PInvoke.WM_INPUTLANGCHANGEREQUEST,
+                PInvoke.INPUTLANGCHANGE_FORWARD,
+                _previousLayout.Value
+            );

Flow.Launcher.Infrastructure/Win32Helper.cs (2)

394-395: Consider capturing process ID for completeness.

The GetWindowThreadProcessId function returns both the thread ID and the process ID via an out parameter. Even if you don't need the process ID right now, capturing it would make the code more complete and maintainable.

-            var threadId = PInvoke.GetWindowThreadProcessId(hwnd);
-            if (threadId == 0) throw new Win32Exception(Marshal.GetLastWin32Error());
+            uint processId;
+            var threadId = PInvoke.GetWindowThreadProcessId(hwnd, out processId);
+            if (threadId == 0) throw new Win32Exception(Marshal.GetLastWin32Error());

---

428-433: Add error handling for message sending.

The code doesn't check if the message was successfully sent or handle potential errors. Consider adding error handling for the message sending operation.

-            PInvoke.PostMessage(
-                hwnd,
-                PInvoke.WM_INPUTLANGCHANGEREQUEST,
-                PInvoke.INPUTLANGCHANGE_FORWARD,
-                _previousLayout.Value
-            );
+            bool success = PInvoke.PostMessage(
+                hwnd,
+                PInvoke.WM_INPUTLANGCHANGEREQUEST,
+                PInvoke.INPUTLANGCHANGE_FORWARD,
+                _previousLayout.Value
+            );
+            
+            if (!success)
+            {
+                // Log error or handle the failure
+                var errorCode = Marshal.GetLastWin32Error();
+                // Could throw or log: new Win32Exception(errorCode)
+            }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Flow.Launcher.Infrastructure/Win32Helper.cs (1)

389-404: 🛠️ Refactor suggestion

Use SendMessage instead of PostMessage for more reliable layout switching

Using PostMessage for keyboard layout restoration creates potential race conditions since it's asynchronous and doesn't wait for the message to be processed. There's also no error handling for the message sending.

Replace with SendMessage to ensure synchronous execution and add error handling:

-            PInvoke.PostMessage(
-                hwnd,
-                PInvoke.WM_INPUTLANGCHANGEREQUEST,
-                PInvoke.INPUTLANGCHANGE_FORWARD,
-                _previousLayout.Value
-            );
+            var result = PInvoke.SendMessage(
+                hwnd,
+                PInvoke.WM_INPUTLANGCHANGEREQUEST,
+                PInvoke.INPUTLANGCHANGE_FORWARD,
+                _previousLayout.Value
+            );
+            
+            if (result.Value == IntPtr.Zero)
+            {
+                var error = Marshal.GetLastWin32Error();
+                if (error != 0)
+                {
+                    System.Diagnostics.Debug.WriteLine($"Failed to restore keyboard layout: {new Win32Exception(error).Message}");
+                }
+            }

Flow.Launcher.Infrastructure/Win32Helper.cs (1)

347-383: Method logic looks good, but add a configuration option

The implementation of SwitchToEnglishKeyboardLayout correctly finds and activates the English keyboard layout. The early exit for IME layouts is also appropriate as it maintains existing behavior.

Consider pulling the IME language tags into a configuration option that can be extended if needed, rather than hardcoding them:

-        private static readonly string[] ImeLanguageTags =
-        {
-            "zh", // Chinese
-            "ja", // Japanese
-            "ko", // Korean
-        };
+        private static readonly string[] ImeLanguageTags = Settings.Instance.ImeLanguageTags ?? new[] 
+        {
+            "zh", // Chinese
+            "ja", // Japanese
+            "ko", // Korean
+        };

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Flow.Launcher.Infrastructure/Win32Helper.cs (2)

389-404: Consider using SendMessage instead of PostMessage for layout restoration

The implementation uses PostMessage which is asynchronous and may introduce race conditions if other keyboard layout changes occur before the message is processed.

-            PInvoke.PostMessage(
-                hwnd,
-                PInvoke.WM_INPUTLANGCHANGEREQUEST,
-                PInvoke.INPUTLANGCHANGE_FORWARD,
-                _previousLayout.Value
-            );
+            PInvoke.SendMessage(
+                hwnd,
+                PInvoke.WM_INPUTLANGCHANGEREQUEST,
+                PInvoke.INPUTLANGCHANGE_FORWARD,
+                _previousLayout.Value
+            );

---

361-363: Capture Process ID for enhanced error handling

The current code only captures and validates the thread ID. For improved error handling and debugging, consider capturing the process ID as well.

-            var threadId = PInvoke.GetWindowThreadProcessId(hwnd);
-            if (threadId == 0) throw new Win32Exception(Marshal.GetLastWin32Error());
+            uint processId;
+            var threadId = PInvoke.GetWindowThreadProcessId(hwnd, out processId);
+            if (threadId == 0) throw new Win32Exception(Marshal.GetLastWin32Error());

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro